refactoring Jason/Mikey RFC#1576
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds RFC 0001 and supporting documentation describing a contract for PowerShell class-based PSDSC resources to participate in DSC v3 semantics via the PowerShell adapter.
Changes:
- Introduces RFC0001 specifying class shape, method signatures, and adapter expectations for class-based PSDSC resources in DSC v3.
- Adds a conceptual guide with examples for authoring class-based PSDSC resources compatible with PSDSC v1/v2 and DSC v3.
- Updates the resources overview to link to the new authoring guide.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| rfc/draft/rfc0001.md | Adds the formal RFC draft defining the proposed contract and adapter behavior. |
| docs/concepts/resources/overview.md | Adds a navigation link to the new class-based PSDSC v3 authoring guide. |
| docs/concepts/resources/class-based-psdsc-v3.md | Adds a conceptual article and example showing how to implement the RFC contract. |
| RFC: RFC0001 # WG will set the number after submission | ||
| Author: jahelmic # <@GitHubUserName> | ||
| Sponsor: michaeltlombardi # <@GitHubUserName> | ||
| Status: Draft # <Draft | Experimental | Accepted | Final> | ||
| SupercededBy: null # <Superceding RFC Number> |
| static [string] InstanceJsonSchema() { | ||
| return ( | ||
| @{ | ||
| '$schema' = 'https://json-schema.org/draft/2020-12/schema' | ||
| type = 'object' | ||
| required = @('name') | ||
| properties = @{ | ||
| name = @{ type = 'string' } | ||
| version = @{ type = 'string' } | ||
| _exist = @{ | ||
| '$ref' = 'https://aka.ms/dsc/schemas/v3/resource/properties/exist.json' | ||
| } | ||
| } | ||
| } | ConvertTo-Json -Depth 10 -Compress |
| if ($null -eq $filteringInstance) { | ||
| throw 'Invalid operation' | ||
| } |
| | Set | `[void] Set()` | `static [void] Set(...)`, `static [<ResourceClass>] Set(...)`, or `static [System.Tuple[<ResourceClass>, String[]]] Set(...)` | | ||
| | Delete | not available | `static [void] Delete([<ResourceClass>]$instance)` | | ||
| | Export | not available | `static [<ResourceClass>[]] Export()` and/or `static [<ResourceClass>[]] Export([<ResourceClass>]$filteringInstance)` | | ||
| | Schema | MOF or class properties | `static [string] InstanceJsonSchema()` | |
There was a problem hiding this comment.
Wouldn't we just want the JSONSchema to be in the adapted manifest?
There was a problem hiding this comment.
I would partially agree here because I think they can be both. I think we should keep the possibility of doing validation through the adapter also. And if this static method is found, it can be used. Makes it an optional escape hatch, but you can also declare constraints that attributes cannot express.
| | --- | --- | --- | | ||
| | Get | `[<ResourceClass>] Get()` | `static [<ResourceClass>] Get([<ResourceClass>]$instance)` | | ||
| | Test | `[bool] Test()` | `static [System.Tuple[bool, <ResourceClass>]] Test(...)` or `static [System.Tuple[bool, <ResourceClass>, String[]]] Test(...)` | | ||
| | Set | `[void] Set()` | `static [void] Set(...)`, `static [<ResourceClass>] Set(...)`, or `static [System.Tuple[<ResourceClass>, String[]]] Set(...)` | |
There was a problem hiding this comment.
How would --what-if work here for both set and delete?
There was a problem hiding this comment.
Just two dedicated methods?
static [System.Tuple[SoftwarePackage, string[]]] WhatIf([SoftwarePackage] $instance) # set preview
static [System.Tuple[SoftwarePackage, string[]]] DeleteWhatIf([SoftwarePackage] $instance) # delete previewI guess it would be addable in an adapted resource manifest, but regardless, the adapter would need a change.
| | Test | `[bool] Test()` | `static [System.Tuple[bool, <ResourceClass>]] Test(...)` or `static [System.Tuple[bool, <ResourceClass>, String[]]] Test(...)` | | ||
| | Set | `[void] Set()` | `static [void] Set(...)`, `static [<ResourceClass>] Set(...)`, or `static [System.Tuple[<ResourceClass>, String[]]] Set(...)` | | ||
| | Delete | not available | `static [void] Delete([<ResourceClass>]$instance)` | | ||
| | Export | not available | `static [<ResourceClass>[]] Export()` and/or `static [<ResourceClass>[]] Export([<ResourceClass>]$filteringInstance)` | |
There was a problem hiding this comment.
There's a PR out to add support for separate schema for export filtering. Reusing the same PS class for export would mean that key still needs to be populated even though it's not needed for export. Alternatively, we could say that the code uses the same type, but any constraints is purely defined in the JSONSchema.
|
@gaelcolas - nice start! You've probably already given it a thought on a base class, but just throwing it in here: class DscResourceBase {
# synthetic compare of current Get() state vs desired ($this).
# Gives every derived resource a correct Test(), free what-if, and pretest question mark?
[bool] Test() {
$current = $this.Get()
foreach ($name in $this.GetConfigurableProperties()) {
$desired = $this.$name
if ($null -eq $desired) { continue } # unspecified property = not enforced
if ($current.$name -ne $desired) { return $false }
}
return $true
}
# Authors MUST override these for a writable resource.
[object] Get() { throw [System.NotImplementedException]::new('Get is not implemented.') }
[void] Set() { throw [System.NotImplementedException]::new('Set is not implemented.') }
# capability ONLY when a derived class overrides the method (see below).
static [object[]] Export() { throw [System.NotImplementedException]::new('Export is not implemented.') }
static [object[]] Export([object] $filter) { throw [System.NotImplementedException]::new('Filtered Export is not implemented.') }
static [void] Delete([object] $instance) { throw [System.NotImplementedException]::new('Delete is not implemented.') }
# Helper available to derived classes: configurable (non-Key, non-NotConfigurable)
# property names, discovered by reflection over [DscProperty()] attributes.
hidden [string[]] GetConfigurableProperties() { <# reflection over $this.GetType() #> }
} |
For @theJasonHelmick and @michaeltlombardi, I've tried to refactor the RFC into something lighter, easier to chew, by moving (maybe aggressively) all related explanations to
docs/in order to keep the RFC concise on the proposal.All parts link back to those docs.
Let me know how that look.